fix(rbac): gate Auditor View on audit:read instead of role string (CS-189)#2581
Merged
fix(rbac): gate Auditor View on audit:read instead of role string (CS-189)#2581
Conversation
… (CS-189)
The Auditor View tab was gated on roles.includes(Role.auditor) — a
literal string match against the built-in "auditor" role. That made the
tab invisible to any custom organization role (e.g. a "Comp AI" custom
role a customer set up for our support team) even when that role was
granted audit:read, and also hid it from owners/admins who implicitly
have the permission. The route layout already enforces audit:read via
requireRoutePermission, so the literal-role gate was both redundant and
wrong.
Switched to canAccessRoute(permissions, 'auditor') in:
- AppSidebar nav item (design-system sidebar)
- MainMenu item (legacy sidebar + mobile menu) — reads permissions via
usePermissions() so callers no longer need to pass hasAuditorRole
- app-shell-search-groups command palette entry
Also removed the redundant roles.includes(Role.auditor) page-level
check in auditor/(overview)/page.tsx; auditor/layout.tsx's existing
requireRoutePermission('auditor', orgId) still enforces audit:read
server-side.
Aligns with CLAUDE.md RBAC guidance: "No manual role string parsing —
always use permission checks."
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…w-up)
Follow-up after CTO review: owner/admin should NOT see the Auditor View
tab or access the page, even though their implicit all-permissions
include audit:read. The tab is scoped to audit work; it should only
appear for users whose role explicitly grants audit access.
New rule:
Show Auditor View iff
- user has the built-in `auditor` role, OR
- user has a custom org role that explicitly grants `audit:read`.
Implementation:
- New pure helper `canAccessAuditorView(roleString, customRolePermissions)`
in lib/permissions.ts. Separates "permission came from built-in role"
(owner/admin/everyone) from "permission came from custom role".
- Server-side helpers in lib/permissions.server.ts:
- `resolveCustomRolePermissions` fetches only the user's custom-role
permissions (built-in roles excluded).
- `resolveAuditorViewAccess` runs the check for the current user.
- `requireAuditorViewAccess` is the route guard — replaces
`requireRoutePermission('auditor', orgId)` on the auditor layout.
- usePermissions() now returns `canAccessAuditorView` + `customPermissions`
so client components don't have to replicate the rule.
- Org layout computes the flag server-side and forwards it through
AppShellWrapper → AppSidebar + getAppShellSearchGroups.
- main-menu reads the flag directly from usePermissions().
Behavior matrix:
- auditor → visible ✅
- owner,auditor / admin,auditor → visible ✅
- CompAI custom role with audit:read → visible ✅
- owner alone / admin alone → hidden (implicit audit:read doesn't count)
- owner,ReadOnlyCustomRole (no audit) → hidden
- employee / contractor → hidden ✅
Adds 9 unit tests locking in every row of that matrix.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/lib/permissions.ts">
<violation number="1" location="apps/app/src/lib/permissions.ts:119">
P1: Auditor View access is checked against custom-role permissions only, which hides the tab for built-in roles with effective `audit:read` (e.g., owner/admin) and diverges from permission-based route gating.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Linked issue analysis
Linked issue: CS-189: [Improvement] Add Auditor View tab to the Comp AI role
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Custom org role with audit:read (e.g., “Comp AI”) should see the Auditor View tab | Added canAccessAuditorView and server-side custom-role resolution |
| ✅ | Auditor View visibility must be based on audit:read permission, not literal 'auditor' role string | Removed literal-role checks and use permission-based helpers |
| ✅ | Auditor View tab should appear/hidden consistently in sidebar, main menu, and command palette | Gated all three surfaces on server/client auditor-view flag |
| ✅ | Auditor View page must render for permitted users (no 404 for custom roles with audit:read) | Removed 404 role check; layout enforces requireAuditorViewAccess |
| ✅ | Follow guidance to avoid manual literal role-string gating when determining tab visibility (use permission checks instead) | Replaced literal role gating with permission helpers and server resolution |
Signed-off-by: Tofik Hasanov <72318342+tofikwest@users.noreply.github.com>
Contributor
Author
|
@cubic-dev-ai review it |
Contributor
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/layout.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/layout.tsx:170">
P2: This adds a second custom-role permission query on every request for custom-role users, duplicating work already done by `resolveUserPermissions` and increasing layout latency.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
…ew gating
Per CS-189 product decision: owners/admins should not see the Auditor View
tab unless they explicitly opt in via a custom role granting audit:read.
Removing audit:read from the built-in owner/admin roles lets the standard
canAccessRoute('auditor') check do the work — the merged permissions
naturally cover owner,auditor / admin,auditor and custom roles with
audit:read, while hiding owner/admin alone.
audit:read is not referenced anywhere in the codebase outside
ROUTE_PERMISSIONS.auditor, so dropping it is safe. Owner/admin still
retain audit:create and audit:update for SOA management.
Deletes ~100 lines of special-case code that was previously needed to
distinguish implicit vs explicit audit:read:
- canAccessAuditorView
- resolveCustomRolePermissions
- resolveAuditorViewAccess
- requireAuditorViewAccess
- customPermissions plumbing through AppShellWrapper / AppSidebar /
search groups / MainMenu / usePermissions
Tests updated to exercise the full resolution path
(resolveBuiltInPermissions + canAccessRoute).
Contributor
There was a problem hiding this comment.
0 issues found across 12 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Linked issue analysis
Linked issue: CS-189: [Improvement] Add Auditor View tab to the Comp AI role
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Gate Auditor View tab in AppSidebar using canAccessRoute(permissions,'auditor') instead of role string check | Updated hidden prop to use canAccessRoute in AppSidebar |
| ✅ | MainMenu: use usePermissions + canAccessRoute and remove hasAuditorRole prop | Imported usePermissions and replaced hasAuditorRole usage |
| ✅ | Command palette (app-shell-search-groups) uses permission check instead of hasAuditorRole | Replaced hasAuditorRole && can('auditor') with can('auditor') |
| ✅ | Remove roles.includes(Role.auditor) check from auditor/(overview)/page.tsx | Deleted roles.includes(Role.auditor) and notFound call |
| ✅ | Drop hasAuditorRole prop from sidebar callsites | Removed hasAuditorRole prop in sidebar.tsx call site |
| ✅ | Remove audit:read from built-in owner/admin roles in packages/auth/src/permissions.ts | Changed audit perms to ['create','update'] for owner/admin |
| ✅ | Add unit tests to lock Auditor View visibility and route mapping | Added 'Auditor View visibility' tests exercising audit:read mapping |
| ✅ | Update training/permissions-regression.spec.ts to reflect owner audit perms change | Adjusted regression test to expect no audit:read for owner |
| Typecheck clean and unit tests pass (CI/test run) as claimed in PR | Tests added but no CI logs or artifacts present in diffs | |
| ✅ | Behavioral: custom role with audit:read sees Auditor View in sidebar, main menu, command palette, and page renders; other roles hidden | UI gates changed + tests show custom role with audit:read passes |
Contributor
Author
|
@cubic-dev-ai review it |
Contributor
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
cubic analysis
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/auth/src/permissions.ts">
<violation number="1" location="packages/auth/src/permissions.ts:77">
P1: According to linked Linear issue CS-189, custom roles (e.g., Comp AI) need Auditor View access, but removing `audit:read` from built-in owner/admin prevents those role managers from granting `audit:read` to custom roles via the privilege-escalation guard.</violation>
</file>
Linked issue analysis
Linked issue: CS-189: [Improvement] Add Auditor View tab to the Comp AI role
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Gate Auditor View tab in AppSidebar using canAccessRoute(permissions,'auditor') instead of role string check | Updated hidden prop to use canAccessRoute in AppSidebar |
| ✅ | MainMenu: use usePermissions + canAccessRoute and remove hasAuditorRole prop | Imported usePermissions and replaced hasAuditorRole usage |
| ✅ | Command palette (app-shell-search-groups) uses permission check instead of hasAuditorRole | Replaced hasAuditorRole && can('auditor') with can('auditor') |
| ✅ | Remove roles.includes(Role.auditor) check from auditor/(overview)/page.tsx | Deleted roles.includes(Role.auditor) and notFound call |
| ✅ | Drop hasAuditorRole prop from sidebar callsites | Removed hasAuditorRole prop in sidebar.tsx call site |
| ✅ | Remove audit:read from built-in owner/admin roles in packages/auth/src/permissions.ts | Changed audit perms to ['create','update'] for owner/admin |
| ✅ | Add unit tests to lock Auditor View visibility and route mapping | Added 'Auditor View visibility' tests exercising audit:read mapping |
| ✅ | Update training/permissions-regression.spec.ts to reflect owner audit perms change | Adjusted regression test to expect no audit:read for owner |
| Typecheck clean and unit tests pass (CI/test run) as claimed in PR | Tests added but no CI logs or artifacts present in diffs | |
| ✅ | Behavioral: custom role with audit:read sees Auditor View in sidebar, main menu, command palette, and page renders; other roles hidden | UI gates changed + tests show custom role with audit:read passes |
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
…ditor View gating" This reverts commit c4a4da5.
Contributor
There was a problem hiding this comment.
0 issues found across 12 files (changes from recent commits).
Requires human review: This PR modifies core RBAC and authorization logic across multiple files, including server-side route guards. Permission changes are high-impact and require human verification.
Contributor
|
🎉 This PR is included in version 3.26.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes CS-189 — customer's custom "Comp AI" org role couldn't see the Auditor View tab, even though the role was granted
audit:read.Investigation
"Comp AI" is not a role that exists in our code. The customer created a custom organization role literally named "Comp AI" (custom roles are stored in
organization_roleper-org). The real bug is broader: the tab-visibility gate usedroles.includes(Role.auditor)— a literal string match against the built-in "auditor" role name. That check hid the tab from:audit:read(including "Comp AI")Meanwhile the route layout (
apps/app/.../auditor/layout.tsx) already enforcesaudit:readviarequireRoutePermission('auditor', orgId), so the literal-role check was both redundant and wrong. This also violates the explicit guidance inCLAUDE.md:Fix
Switched the four places gating the Auditor View tab to
canAccessRoute(permissions, 'auditor'):apps/app/src/app/(app)/[orgId]/components/AppSidebar.tsx(design-system sidebar)apps/app/src/components/main-menu.tsx(legacy sidebar + mobile menu) — now reads permissions viausePermissions()so callers don't need to passhasAuditorRoleapps/app/src/app/(app)/[orgId]/components/app-shell-search-groups.tsx(command palette)apps/app/src/app/(app)/[orgId]/auditor/(overview)/page.tsx— removed redundantroles.includes(Role.auditor)check (layout still enforcesaudit:read)Also dropped the now-unused
hasAuditorRoleprop fromMainMenu/sidebar.tsxcall sites. The separateisOnlyAuditorconcept (for hiding non-auditor tabs for external auditors) is untouched.Behavioral impact
audit:read→ tab visible ✅ (was hidden; fixes the ticket)auditorrole → tab visible ✅ (unchanged)audit:read→ tab hidden ✅ (unchanged)Test plan
permissions.test.tslock in theauditorroute mapping (any role withaudit:readpasses; missingaudit:readfails)npx vitest run src/lib/permissions.test.ts— 23/24 pass (1 failure is pre-existing, unrelated to this change)audit:read, Auditor View appears in the sidebar, main menu, and command palette, and the page rendersemployeerole, tab is hidden in all three surfacesauditorrole, behavior unchanged🤖 Generated with Claude Code
Summary by cubic
Scope Auditor View to explicit auditor access: show it only for the built-in
auditorrole or custom roles withaudit:read; hide it from owners/admins unless explicitly granted. Fixes CS‑189 by letting the customer’s “Comp AI” custom role see the tab and page.Bug Fixes
canAccessAuditorViewin AppSidebar,MainMenu(viausePermissions()), and command palette; drophasAuditorRolefromMainMenuand its call site.requireAuditorViewAccessinauditor/layout.tsx; remove the redundant role check inauditor/(overview)/page.tsx.canAccessAuditorViewthroughAppShellWrapperto the sidebar and search groups.Refactors
canAccessAuditorView,resolveCustomRolePermissions,resolveAuditorViewAccess, andrequireAuditorViewAccess; exposecustomPermissionsandcanAccessAuditorViewfromusePermissions().audit:read.Written for commit 2573c20. Summary will update on new commits.